Remove eval from defcomp macro #761
Conversation
Codecov Report
@@ Coverage Diff @@
## master #761 +/- ##
==========================================
- Coverage 77.91% 77.82% -0.10%
==========================================
Files 36 36
Lines 2998 2994 -4
==========================================
- Hits 2336 2330 -6
- Misses 662 664 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
sorry I'm late to seeing this after you merged it-- but I see that you had to change some of the tests for defaults. Did you make sure that the changed behavior makes sense? Cause it looks like you moved the code for checking the number of dimensions for a default value, and codecov says that those modified lines aren't being hit by the tests, which is supposed to be the test that you modified. does that make sense? |
| if length(dimensions) != ndims(default) | ||
| error("Default value has different number of dimensions ($(ndims(dflt))) than parameter '$name' ($(length(dimensions)))") | ||
| end |
There was a problem hiding this comment.
ah actually it is just the "end" line of the if statement that codecov is complaining about. But I was surprised that the error type for that error would be UndefVarError and not still a LoadError, so I think there might be a problem here that dflt isn't defined here and that it should be default?
There was a problem hiding this comment.
Ah, yes, good catch! The test is now reporting a bug in the line that throws the error, i.e. in line 87 :) The test should actually test for an ErrorException, right? That is what is thrown, so any other type of error would mean something went wrong.
There was a problem hiding this comment.
Good point, I was lazy when I changed that, I’ll change the error type back and correct dflt to default, I think that should solve it.
There was a problem hiding this comment.
Sorry, but did we conclude that it should be a LoadError or ErrorException? It's now an ErrorException (see #762 ), but even with default instead of dflt. Is there a problem here because we're calling ndims on default before it is evaluated ... or has it been evaluated at this point?
There was a problem hiding this comment.
I think David is right, it should now be ErrorException (it was previously LoadError when it was being thrown from within eval I think)
There was a problem hiding this comment.
Also if you just run the part of the test file that's supposed to test this error, but run it outside of the test_throws then you can see if the error message is our constructed error message or if something else is wrong, just to be safe
There was a problem hiding this comment.
@ckingdon95 smart yes I just did that and the constructed error message looks correct, so I think we can merge the PR now if someone can approve it
#760